Guard admin endpoints behind auth by default by ChristianPavilonis · Pull Request #443 · IABTechLab/trusted-server

ChristianPavilonis · 2026-03-05T18:33:52Z

Summary

Fix security vulnerability where /admin/keys/rotate and /admin/keys/deactivate were publicly accessible when no handler regex covered /admin/* paths
Settings::from_toml_and_env() now returns a hard error when no handler covers admin endpoints, catching misconfiguration at build time
enforce_basic_auth remains a single, simple mechanism — admin protection is enforced by requiring handler coverage in the configuration

Changes

File	Change
`crates/common/src/auth.rs`	Simplify `enforce_basic_auth` — remove `is_admin_path()` and runtime admin guard; admin paths use standard handler-based auth
`crates/common/src/settings.rs`	Add build-time validation: `from_toml_and_env()` errors when `uncovered_admin_endpoints()` finds unprotected admin paths; update doc comments and tests
`crates/common/build.rs`	Remove `cargo:warning` loop (redundant — `from_toml_and_env` now errors on uncovered admin endpoints)
`crates/common/src/test_support.rs`	Add `^/admin` handler to base test settings
`trusted-server.toml`	Add `^/admin` handler to dev config

Closes

Closes #400

Test plan

cargo test --workspace (472 tests pass)
cargo clippy --all-targets --all-features -- -D warnings
cargo fmt --all -- --check

Checklist

Changes follow CLAUDE.md conventions
No unwrap() in production code — use expect("should ...")
Uses tracing macros (not println!)
New code has tests
No secrets or credentials committed

aram356

Summary

This PR correctly identifies and fixes the security gap where admin endpoints could be accessed without authentication when no handler is configured. The fail-closed approach is sound. However, the implementation adds complexity that may not be necessary given the existing handler-based auth system.

♻️ refactor

Simplify: use config validation instead of runtime guard: The handler-based auth already enforces credentials correctly. Instead of adding is_admin_path() and special-case logic in enforce_basic_auth, make Settings::validate() return an error when no handler covers admin endpoints. This keeps enforce_basic_auth unchanged, eliminates the dual-mechanism (is_admin_path prefix check vs ADMIN_ENDPOINTS list), and catches misconfiguration at build time as an error rather than silently returning 401s at runtime. The existing uncovered_admin_endpoints() method is the right building block — call it from validate() instead of build.rs.

aram356 · 2026-03-06T06:56:47Z

To clarify the refactor suggestion above — the required config is just a standard [[handlers]] entry in trusted-server.toml:

[[handlers]]
path = "^/admin"
username = "admin"
password = "a-strong-password"

With the validation approach, Settings::validate() would error at build time if this entry is missing, ensuring admin endpoints are always covered by the existing handler-based auth — no runtime special-casing needed.

ChristianPavilonis · 2026-03-06T15:44:13Z

Addressed the review feedback — refactored from runtime guard to build-time config validation:

Removed is_admin_path() and the special-case admin branch from enforce_basic_auth — it's back to a single, simple mechanism
Added a hard error in Settings::from_toml_and_env() when uncovered_admin_endpoints() returns any uncovered paths — build fails immediately with a clear message
Removed the cargo:warning loop in build.rs (redundant now that from_toml_and_env errors)
Added ^/admin handler to trusted-server.toml and test fixtures
Added from_toml_and_env_rejects_config_without_admin_handler test

All 472 tests pass, clippy clean, fmt clean.

Admin paths (/admin/*) were only auth-gated when a configured handler's path regex happened to match them. The default config (^/secure) left /admin/keys/rotate and /admin/keys/deactivate publicly accessible. Now enforce_basic_auth always denies admin paths that no handler covers, and a startup warning alerts operators when no handler matches /admin/. Closes #400

P2: Check all admin endpoints (rotate + deactivate) in coverage warning instead of only checking rotate. P2: Move admin coverage warning from per-request main() to build time via cargo:warning in build.rs, avoiding log noise in Fastly Compute where every request is a fresh WASM instance. P3: Guard exact /admin path in addition to /admin/ prefix so future endpoints at that path are also protected.

Move admin endpoint protection from a runtime check in enforce_basic_auth to a build-time validation error in Settings::from_toml_and_env. This eliminates the dual-mechanism (is_admin_path prefix check vs handler-based auth) and catches misconfiguration at build time rather than silently returning 401s at runtime.

aram356

Summary

Build-time config validation for admin endpoints is the right approach — misconfiguration becomes a compile error. The implementation is clean and well-tested. Two items need addressing before merge.

Blocking

🔧 wrench

from_toml() bypasses admin validation: from_toml() is pub and skips the uncovered_admin_endpoints() check entirely. Defense-in-depth: add the same check to from_toml() so no code path can construct a Settings without admin coverage. Cost is one trivial Vec allocation. See inline comment on settings.rs:377.
ADMIN_ENDPOINTS can drift from router: The hardcoded list has no compile-time or test-time link to the actual route table in main.rs:97-98. A new admin route added without updating this constant ships unprotected. See inline comment on settings.rs:404.

Non-blocking

🌱 seedling

Placeholder changeme password: No validation rejects weak handler passwords, unlike the synthetic secret key check. (trusted-server.toml:9)

♻️ refactor

Nested temp_env::with_var: 7-level nesting can be flattened with temp_env::with_vars. (crates/common/src/settings.rs:841)

🤔 thinking

uncovered_admin_endpoints visibility: pub is broader than needed — pub(crate) would suffice. (crates/common/src/settings.rs:412)

⛏ nitpick

Comment says "fail" but means "panic": .expect() panics, not returns an error. (crates/common/build.rs:31)

👍 praise

Build-time validation approach: Shifting auth enforcement to config validation at compile time is architecturally sound. The uncovered_admin_endpoints method and its test coverage (full/partial/no coverage) are thorough.

CI Status

cargo fmt: PASS
cargo test: PASS
vitest: PASS
format-typescript: PASS
format-docs: PASS
CodeQL: PASS

crates/common/src/settings.rs

trusted-server.toml

crates/common/src/settings.rs

crates/common/build.rs

crates/common/src/settings.rs

Add admin endpoint validation to from_toml() so no code path can construct Settings without admin coverage. Add a compile-time sync test that verifies ADMIN_ENDPOINTS matches the Fastly router source. Also flatten nested temp_env::with_var calls, narrow visibility of uncovered_admin_endpoints to pub(crate), and fix build.rs comment.

ChristianPavilonis · 2026-03-10T19:36:46Z

Review feedback addressed

All comments from the second review have been addressed in 0ffad1b:

Blocking

from_toml() bypasses admin validation — Fixed. from_toml() now runs the same uncovered_admin_endpoints() check as from_toml_and_env(). Updated all test TOMLs (in google_tag_manager.rs, prebid.rs) to include a [[handlers]] entry covering /admin. Tests that deliberately exercise uncovered_admin_endpoints() on invalid configs use toml::from_str directly to bypass validation.
ADMIN_ENDPOINTS can drift from router — Added admin_endpoints_match_fastly_router sync test in crates/common/src/settings.rs. It uses include_str! to read crates/fastly/src/main.rs at compile time and asserts both directions: every ADMIN_ENDPOINTS entry exists in the router, and every /admin/ route in the router exists in ADMIN_ENDPOINTS. Also added a cross-reference comment at the router match arms.

Non-blocking

Nested temp_env::with_var — Flattened 7-level nesting to a single temp_env::with_vars call.
uncovered_admin_endpoints visibility — Changed both ADMIN_ENDPOINTS and uncovered_admin_endpoints() to pub(crate).
Comment says "fail" but means "panic" — Fixed in build.rs.

All 474 tests pass, clippy clean, fmt clean.

ChristianPavilonis self-assigned this Mar 5, 2026

ChristianPavilonis requested review from aram356 and prk-Jr March 5, 2026 19:25

aram356 reviewed Mar 6, 2026

View reviewed changes

ChristianPavilonis requested a review from aram356 March 6, 2026 15:46

ChristianPavilonis added 3 commits March 9, 2026 11:56

ChristianPavilonis force-pushed the feature/guard-admin-endpoints branch from e3dd80c to 59a036e Compare March 9, 2026 16:56

aram356 requested changes Mar 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guard admin endpoints behind auth by default#443

Guard admin endpoints behind auth by default#443
ChristianPavilonis wants to merge 4 commits intomainfrom
feature/guard-admin-endpoints

ChristianPavilonis commented Mar 5, 2026 •

edited

Loading

Uh oh!

aram356 left a comment •

edited

Loading

Uh oh!

aram356 commented Mar 6, 2026

Uh oh!

ChristianPavilonis commented Mar 6, 2026

Uh oh!

aram356 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristianPavilonis commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ChristianPavilonis commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Closes

Test plan

Checklist

Uh oh!

aram356 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Summary

♻️ refactor

Uh oh!

aram356 commented Mar 6, 2026

Uh oh!

ChristianPavilonis commented Mar 6, 2026

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

Non-blocking

🌱 seedling

♻️ refactor

🤔 thinking

⛏ nitpick

👍 praise

CI Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristianPavilonis commented Mar 10, 2026

Review feedback addressed

Blocking

Non-blocking

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ChristianPavilonis commented Mar 5, 2026 •

edited

Loading

aram356 left a comment •

edited

Loading